Remote Runner / PostMessge api #456
Conversation
flashdesignory
requested review from
bgrins,
camillobruni,
rniwa,
julienw and
smaug----
There was a problem hiding this comment.
I haven't tried locally but this looks pretty good to me. We can always fix things later if there's anything broken anyway.
r+ with my comments fixed.
Note: it would be a lot easier to review if you'd reworked the PR to have consistent commits, for example:
Commit 1: move files around without changing them, just update the references in other files
Commit 2: add the remote runner + the workload tools
Commit 3: change the workload itself
If you do this for future complex PR this will make them a joy to review (and hence faster to review as well). Thanks!
|
|
||
| export default function App() { | ||
| useLayoutEffect(() => { | ||
| connectToBenchmark(getBenchmarkSuitesManager(), "news-next", 1); |
There was a problem hiding this comment.
I admit I haven't followed closely, but do we need to disconnect when it's unmounted?
There was a problem hiding this comment.
added a cleanup function. It's less important here, but I am now also disconnecting after the "suite-complete' message has been sent.
| @@ -0,0 +1,41 @@ | |||
| import { BenchmarkTestStep, BenchmarkTestSuite, BenchmarkSuitesManager, forceLayout, getElement } from "speedometer-utils/workload-testing-utils.mjs"; | |||
There was a problem hiding this comment.
oh I so want #371 to be merged finally
| get frame() { | ||
| return this.#frame; | ||
| } | ||
|
|
||
| get page() { | ||
| return this.#page; | ||
| } | ||
|
|
||
| get params() { | ||
| return this.#params; | ||
| } | ||
|
|
||
| get suite() { | ||
| return this.#suite; | ||
| } | ||
|
|
||
| get client() { | ||
| return this.#client; | ||
| } | ||
|
|
||
| get suiteResults() { | ||
| return this.#suiteResults; | ||
| } | ||
|
|
There was a problem hiding this comment.
frankly all these private variables look superfluous to me, I'd rather put everything public, but well I'm not opposed either.
There was a problem hiding this comment.
I feel like this should be a discussion point outside of the current prs.
| * Various methods that are extracted from the Page class. | ||
| */ |
There was a problem hiding this comment.
could the Page class use them directly (maybe as a follow-up, I don't know)
I don't feel comfortable with the duplication.
There was a problem hiding this comment.
Agreed - This should be a follow up!
resources/suite-runner.mjs
Outdated
| this.suiteResults.tests = response.result.tests; | ||
| this.suiteResults.total += response.result.total; |
There was a problem hiding this comment.
why are we overriding the tests property but incrementing the total value?
Should we append the tests instead?
(I haven't researched closely but it would be good to comment about that)
There was a problem hiding this comment.
cleaned this up - thank you!
| @@ -146,21 +146,25 @@ class Params { | |||
| return shuffleSeed; | |||
| } | |||
|
|
|||
| toSearchParams() { | |||
| toSearchParams(forRemote = false) { | |||
There was a problem hiding this comment.
can we have an option object parameter instead of a boolean? it's really not clear what this does from the caller site.
(and actually from here neither, a comment would be nice)
There was a problem hiding this comment.
I added some comments in that function.
We don't use option params in our current code base and it feels too much, just for readability.
We can rename "forRemote" or I can also add a comment on the caller site, if that helps.
There was a problem hiding this comment.
a comment on the caller site would be good, thanks
| return new URLSearchParams(rawParams).toString(); | ||
| } | ||
| } | ||
|
|
||
| export const defaultParams = new Params(); | ||
|
|
||
| const searchParams = new URLSearchParams(window.location.search); | ||
| const searchParams = new URLSearchParams(typeof window !== "undefined" ? window.location.search : undefined); |
There was a problem hiding this comment.
when is it happening that window isn't present?
optional nit: use ?. operator
There was a problem hiding this comment.
Next.js throws an error during the build process.
I did try the ?. operator, but the current implementation was the only way to make Next.js stop complaining.
Co-authored-by: Julien Wajsberg <[email protected]>
|
Still looking good from my side. |
This pr introduces Remote Runner workloads.
Workloads that are local OR that point to a remote url (when running Speedometer locally), can communicate and run their own tests via PostMessage.
Basic flow:
Notes